Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "buffer: convert offset & length to int properly" #9814

Closed
wants to merge 1 commit into from
Closed

Revert "buffer: convert offset & length to int properly" #9814

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 26, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
Affected core subsystem(s)

buffer

Description of change

This reverts commit ca37fa5.

The commit was landed without running through CI. A test provided by the commit fails on most (but not all) platforms on CI. See, for example, https://ci.nodejs.org/job/node-test-commit-linux/6250/nodes=centos5-32/console:

not ok 24 parallel/test-buffer-creation-regression
  ---
  duration_ms: 0.248
  severity: fail
  stack: |-
    /home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:7
      const arrayBuffer = new ArrayBuffer(size);
                          ^
    
    RangeError: Invalid array buffer length
        at new ArrayBuffer (<anonymous>)
        at test (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:7:23)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:21:1)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)

Refs: #9492

@Trott Trott added the buffer Issues and PRs related to the buffer subsystem. label Nov 26, 2016
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 26, 2016
@Trott Trott mentioned this pull request Nov 26, 2016
Closed
3 tasks
@Trott
Copy link
Member Author

Trott commented Nov 26, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/4989/

@Trott
Copy link
Member Author

Trott commented Nov 26, 2016

One failure on Linux is infrastructure-related, but let's re-run anyway (Linux-only): https://ci.nodejs.org/job/node-test-commit-linux/6252/

@Trott
Copy link
Member Author

Trott commented Nov 26, 2016

Linux re-run is green, rest of first CI is also green.

I think this warrants landing Real Soon Now rather than waiting the 72 hours, but I'd feel better getting a 👍 on that from a CTC person or two. Any takers?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 26, 2016

Go for it!

@ChALkeR
Copy link
Member

ChALkeR commented Nov 26, 2016

/cc @thefourtheye — it looks like #9492 should be re-done after this lands.

Trott added a commit to Trott/io.js that referenced this pull request Nov 26, 2016
@Trott
Revert "buffer: convert offset & length to int properly"
6b2aa1a 
This reverts commit ca37fa5.

A test provided by the commit fails on most (but not all) platforms on
CI.

PR-URL: nodejs#9814
Ref: nodejs#9492
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 26, 2016

Landed in 6b2aa1a. Thanks.

@Trott Trott closed this Nov 26, 2016
@brodycj
Copy link

brodycj commented Nov 26, 2016

As a nit: I am happy to see the problematic test (test\parallel\test-buffer-creation-regression.js) but not the whole change reverted. The change does seem to solve a problem, it does not break any other tests, and the other 2 tests seem to pass OK if I am not mistaken here.

I think a better solution would have been to simply skip the "problematic"test, and better yet don't skip the "problematic" test on certain platform(s) where we know it will pass (macOS which is known to be 64-bit UNIX, for example). I think this would have kept the project history a little cleaner. Just a suggestion for the future.

/cc @thefourtheye

@MylesBorins
Copy link
Contributor

@brodybits in general when we revert we do so of the entire atomic change. As the original change did not go through CI I think it makes a bunch of sense to revert the entire thing.

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Nov 27, 2016
@thefourtheye
buffer: convert offset & length to int properly
884eb35 
As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
`offset` would be an integer, not a 32 bit unsigned integer. Also,
`length` would be an integer with the maximum value of 2^53 - 1, not a
32 bit unsigned integer.

This would be a problem because, if we create a buffer from an
arraybuffer, from an offset which is greater than 2^32, it would be
actually pointing to a different location in arraybuffer. For example,
if we use 2^40 as offset, then the actual value used will be 0,
because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
unsigned int, which is based on 2^32 modulo.

This is a redo, as the ca37fa5 broke
CI.

Refer: nodejs#9814
Refer: nodejs#9492
@thefourtheye thefourtheye mentioned this pull request Nov 27, 2016
Closed
3 tasks
@thefourtheye
Copy link
Contributor

I am extremely sorry for the trouble I created. Thanks @Trott for fixing this immediately.

thefourtheye added a commit that referenced this pull request Dec 5, 2016
@thefourtheye
buffer: convert offset & length to int properly
720d01f 
As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
`offset` would be an integer, not a 32 bit unsigned integer. Also,
`length` would be an integer with the maximum value of 2^53 - 1, not a
32 bit unsigned integer.

This would be a problem because, if we create a buffer from an
arraybuffer, from an offset which is greater than 2^32, it would be
actually pointing to a different location in arraybuffer. For example,
if we use 2^40 as offset, then the actual value used will be 0,
because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
unsigned int, which is based on 2^32 modulo.

This is a redo, as the ca37fa5 broke
CI.

Refer: #9814
Refer: #9492

PR-URL: #9815

Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Dec 5, 2016
@thefourtheye @Fishrock123
buffer: convert offset & length to int properly
0c745e3 
As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
`offset` would be an integer, not a 32 bit unsigned integer. Also,
`length` would be an integer with the maximum value of 2^53 - 1, not a
32 bit unsigned integer.

This would be a problem because, if we create a buffer from an
arraybuffer, from an offset which is greater than 2^32, it would be
actually pointing to a different location in arraybuffer. For example,
if we use 2^40 as offset, then the actual value used will be 0,
because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
unsigned int, which is based on 2^32 modulo.

This is a redo, as the ca37fa5 broke
CI.

Refer: #9814
Refer: #9492

PR-URL: #9815

Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@thefourtheye
buffer: convert offset & length to int properly
9c3240b 
As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
`offset` would be an integer, not a 32 bit unsigned integer. Also,
`length` would be an integer with the maximum value of 2^53 - 1, not a
32 bit unsigned integer.

This would be a problem because, if we create a buffer from an
arraybuffer, from an offset which is greater than 2^32, it would be
actually pointing to a different location in arraybuffer. For example,
if we use 2^40 as offset, then the actual value used will be 0,
because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
unsigned int, which is based on 2^32 modulo.

This is a redo, as the ca37fa5 broke
CI.

Refer: nodejs#9814
Refer: nodejs#9492

PR-URL: nodejs#9815

Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@Trott @italoacasas
Revert "buffer: convert offset & length to int properly"
eea08a5 
This reverts commit ca37fa5.

A test provided by the commit fails on most (but not all) platforms on
CI.

PR-URL: nodejs#9814
Ref: nodejs#9492
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@thefourtheye thefourtheye mentioned this pull request Feb 5, 2017
Closed
3 tasks
jasnell pushed a commit that referenced this pull request Mar 3, 2017
@thefourtheye @jasnell
buffer: convert offset & length to int properly
82a008b 
  As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
  `offset` would be an integer, not a 32 bit unsigned integer. Also,
  `length` would be an integer with the maximum value of 2^53 - 1, not a
  32 bit unsigned integer.

  This would be a problem because, if we create a buffer from an
  arraybuffer, from an offset which is greater than 2^32, it would be
  actually pointing to a different location in arraybuffer. For example,
  if we use 2^40 as offset, then the actual value used will be 0,
  because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
  unsigned int, which is based on 2^32 modulo.

  This is a redo, as the ca37fa5 broke
  CI.

  Refer: #9814
  Refer: #9492

  PR-URL: #9815

  Reviewed-By: Michaël Zasso <targos@protonmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-Of: #9815
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@thefourtheye @MylesBorins
buffer: convert offset & length to int properly
f83d035 
  As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
  `offset` would be an integer, not a 32 bit unsigned integer. Also,
  `length` would be an integer with the maximum value of 2^53 - 1, not a
  32 bit unsigned integer.

  This would be a problem because, if we create a buffer from an
  arraybuffer, from an offset which is greater than 2^32, it would be
  actually pointing to a different location in arraybuffer. For example,
  if we use 2^40 as offset, then the actual value used will be 0,
  because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit
  unsigned int, which is based on 2^32 modulo.

  This is a redo, as the ca37fa5 broke
  CI.

  Refer: #9814
  Refer: #9492

  PR-URL: #9815

  Reviewed-By: Michaël Zasso <targos@protonmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Backport-Of: #9815
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@Trott Trott deleted the revert9492 branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @cjihrig @ChALkeR @brodycj @MylesBorins @thefourtheye @bnoordhuis @targos @addaleax @nodejs-github-bot